Edit feature for facilities in organization | pincode, geo_organization info auto populates #9662
Conversation
WalkthroughThis pull request introduces significant changes to the facility management workflow, primarily focusing on the removal of the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
On facility edit the organisation is also not getting prefilled, I think we should solve that also |
You mean |
|
I don't think jeevan is working on it, I will assign it to you. lets have one PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/pages/Organization/components/OrganizationSelector.tsx (1)
35-55: Enhance defensive checks around JSON parsing.
Currently, atry/catchblock is in place to catchJSON.parseerrors, logging them to the console. Consider adding a fallback path or early return to ensure the component’s state remains stable in edge cases (e.g., an empty string or invalid structure) to avoid undesired behavior.useEffect(() => { if (value) { try { const parsedValue = typeof value === "string" ? JSON.parse(value) : value; if (parsedValue) { // ... } } catch (e) { + setSelectedLevels([]); console.error("Invalid value for geo_organization:", e); } } }, [value]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json(1 hunks)src/components/Facility/FacilityCreate.tsx(4 hunks)src/pages/Organization/components/OrganizationSelector.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Facility/FacilityCreate.tsx
🔇 Additional comments (4)
src/pages/Organization/components/OrganizationSelector.tsx (3)
2-2: Import statement is correctly updated.
No issues found with the import of React hooks.
31-31: Prop destructuring for newvalueis fine.
Thevalueprop is properly introduced here, and the necessary props are being destructured clearly.
171-171: No issues with the Autocomplete value assignment.
Using the last selected organization’sidor an empty string is a sensible approach.public/locale/en.json (1)
1584-1584: New i18n key added.
The"select_location_from": "Select location from"entry aligns with the updated UI text references. Ensure that its usage is consistent across the app, and consider whether this prompt needs additional context or placeholders.
Mahendar0701
changed the title
|
the issue still presist, can you attach a video after fixing the issue |
Mahendar0701
changed the title
|
edit may work fine, but is it auto-filling when pincode is entered? |
Nope , i will fix that too 👍 |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/Facility/FacilityCreate.tsx (1)
224-232:⚠️ Potential issueMove error handling to useEffect.
The current error handling can trigger multiple toasts during renders. Move it to useEffect for better user experience.
-if (isPincodeError) { - toast.error("Invalid pincode"); -} +useEffect(() => { + if (isPincodeError) { + toast.error(t("invalid_pincode")); + } + return () => { + toast.dismiss(); + }; +}, [isPincodeError, t]);
🧹 Nitpick comments (1)
src/components/Facility/FacilityCreate.tsx (1)
234-250: Add loading states for organization selection.The organization selection logic should handle loading states to provide better feedback to users.
+const [isLoadingOrg, setIsLoadingOrg] = useState(false); + useEffect(() => { + setIsLoadingOrg(true); if (stateOrg && districtOrg) { setSelectedLevels([stateOrg, districtOrg]); } else { setSelectedLevels([]); } + setIsLoadingOrg(false); }, [stateOrg, districtOrg]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json(10 hunks)src/components/Facility/FacilityCreate.tsx(9 hunks)src/pages/Organization/components/OrganizationSelector.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pages/Organization/components/OrganizationSelector.tsx
- public/locale/en.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (4)
src/components/Facility/FacilityCreate.tsx (4)
1-1: LGTM! Well-structured imports and state management.The new imports and state variables are properly organized and typed, supporting the pincode auto-fill and organization selection features.
Also applies to: 63-66, 75-77
158-158: LGTM! Proper form reset handling.The form reset logic correctly handles the geo_organization field, ensuring it's properly populated during facility editing.
513-515: LGTM! Well-structured visibility settings section.The visibility settings section is properly internationalized and follows the UI pattern consistently.
375-379: Verify organization prefill functionality.Based on the PR comments, please verify that the organization prefill works correctly when editing an existing facility.
✅ Verification successful
Organization prefill implementation is correct
The code properly handles organization prefill in edit mode through the useEffect hook that initializes the form with facility data, including the geo_organization field. The watch and setValue implementations in the reviewed code correctly maintain the field's state.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if organization field is properly handled in edit mode ast-grep --pattern 'useEffect(() => { if (facilityData) { $$$ geo_organization: $$$ $$$ } }, $$$)'Length of output: 116
Script:
#!/bin/bash # Search for form initialization with facilityData rg "facilityData" -A 5 -B 5 "src/components/Facility/FacilityCreate.tsx" # Also check for form default values ast-grep --pattern 'defaultValues: { $$$ }'Length of output: 1731
| description: facilityData.description || "", | ||
| features: facilityData.features || [], | ||
| pincode: facilityData.pincode?.toString() || "", | ||
| geo_organization: facilityData.geo_organization || "", |
There was a problem hiding this comment.
geo_organization here would be an Organization object rather than a string.
Update return type for getPermittedFacility to be FacilityData rather than FacilityModel (make any additional changes as needed when updating this route).
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
src/pages/Organization/components/EditFacilitySheet.tsx (2)
27-27: Remove debug console.log statement.Debug logging should be removed before merging to production.
- console.log(open);
19-22: Remove unused organizationId prop.The
organizationIdprop is defined in the Props interface but not used in the component.interface Props { - organizationId?: string; facilityId: string; }src/pages/Appointments/components/AppointmentTokenCard.tsx (1)
Line range hint
71-72: Address TODO comment about token number.The comment indicates that the token number should come from the backend instead of using
getFakeTokenNumber.Would you like me to help create a GitHub issue to track the implementation of backend token number support?
src/components/Facility/FacilityForm.tsx (2)
Line range hint
1-58: Consider enhancing schema validation for geo_organization.The current validation only checks for non-empty strings. Consider adding more specific validation rules to ensure the value is a valid organization reference.
- geo_organization: z.string().min(1, { message: "required" }), + geo_organization: z.string().min(1, { message: "required" }).refine( + (val) => { + try { + const parsed = JSON.parse(val); + return parsed && typeof parsed === 'object'; + } catch { + return false; + } + }, + { message: "Invalid organization format" } + ),
372-381: Add error handling for organization selection.Consider adding error handling for cases where the organization selection fails or becomes invalid.
<OrganizationSelector required={true} value={form.watch("geo_organization")} parentSelectedLevels={selectedLevels} onChange={(value) => { + try { form.setValue("geo_organization", value); + } catch (error) { + console.error("Failed to update organization:", error); + toast.error(t("organization_update_failed")); + } }} />src/components/Facility/FacilityCreate.tsx (1)
375-379: Add validation before setting organization value.Consider validating the organization value before setting it in the form.
value={form.watch("geo_organization")} parentSelectedLevels={selectedLevels} onChange={(value) => { + if (!value) { + toast.error(t("invalid_organization_selection")); + return; + } form.setValue("geo_organization", value); }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
public/locale/en.json(10 hunks)src/Utils/featureFlags.tsx(2 hunks)src/Utils/request/api.tsx(1 hunks)src/Utils/utils.ts(2 hunks)src/components/Facility/FacilityCreate.tsx(9 hunks)src/components/Facility/FacilityForm.tsx(12 hunks)src/components/Facility/FacilityHome.tsx(3 hunks)src/pages/Appointments/AppointmentDetail.tsx(2 hunks)src/pages/Appointments/components/AppointmentTokenCard.tsx(1 hunks)src/pages/Appointments/utils.ts(2 hunks)src/pages/Organization/OrganizationFacilities.tsx(2 hunks)src/pages/Organization/components/AddFacilitySheet.tsx(3 hunks)src/pages/Organization/components/EditFacilitySheet.tsx(1 hunks)src/pages/Organization/components/OrganizationSelector.tsx(4 hunks)src/types/facility/facility.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Utils/utils.ts
- src/pages/Organization/components/AddFacilitySheet.tsx
- src/pages/Organization/components/OrganizationSelector.tsx
- public/locale/en.json
🔇 Additional comments (14)
src/types/facility/facility.ts (1)
31-35: LGTM! Well-structured type definitions.The new optional properties
locationandfacility_flagsare properly typed and maintain backward compatibility.src/pages/Organization/components/EditFacilitySheet.tsx (1)
47-53: LGTM! Proper query invalidation.Good practice to invalidate the currentUser query after successful form submission to ensure UI reflects the latest data.
src/Utils/featureFlags.tsx (1)
50-53: LGTM! Consistent type updates.The type changes from FacilityModel to FacilityData are consistent with the type definitions and maintain type safety throughout the feature flags system.
src/pages/Appointments/components/AppointmentTokenCard.tsx (2)
10-10: LGTM! Consistent type updates.The type change from FacilityModel to FacilityData is consistent with the type system updates.
Also applies to: 16-16
Line range hint
47-53: Verify auto-population functionality.Based on PR comments, there are concerns about:
- Organization info not being prefilled
- Auto-fill not working when pincode is entered
Let's verify the implementation:
src/pages/Appointments/utils.ts (1)
144-144: LGTM! Type update aligns with codebase refactoring.The change from
FacilityModeltoFacilityDatais consistent with the broader type refactoring across the codebase.src/Utils/request/api.tsx (1)
199-199: LGTM! API response type update is consistent.The update of the
getPermittedFacilityroute's response type toFacilityDataaligns with the codebase's type system refactoring.src/pages/Appointments/AppointmentDetail.tsx (1)
210-210: LGTM! Props type update is consistent.The change from
FacilityModeltoFacilityDatain theAppointmentDetailscomponent props aligns with the type system refactoring.src/pages/Organization/OrganizationFacilities.tsx (4)
20-20: LGTM!The import statement follows the project's import order convention.
118-120: LGTM! Key prop added as requested.The change addresses the previous review comment by adding a unique
keyprop to theCardcomponent.
122-148: LGTM! Well-structured facility card implementation.The code follows best practices with:
- Proper conditional rendering of facility images
- Fallback to avatar when no image is available
- Clear organization of facility information
149-172: LGTM! Accessible card footer implementation.The implementation:
- Properly separates edit and view actions
- Uses semantic HTML structure
- Follows accessibility best practices with proper button and link usage
src/components/Facility/FacilityCreate.tsx (2)
224-232:⚠️ Potential issueMove error handling to useEffect to prevent render issues.
The current implementation shows toast errors during render, which can cause multiple notifications and side effects.
- if (isPincodeError) { - toast.error("Invalid pincode"); - } + useEffect(() => { + if (isPincodeError) { + toast.error(t("invalid_pincode")); + } + return () => { + toast.dismiss(); + }; + }, [isPincodeError, t]);Likely invalid or redundant comment.
234-250: 🛠️ Refactor suggestionEnhance error handling for missing organization data.
Add proper error handling when state or district organizations are not found.
useEffect(() => { if (stateOrg && districtOrg) { setSelectedLevels([stateOrg, districtOrg]); } else { setSelectedLevels([]); + if (stateName && !stateOrg) { + toast.error(t("state_not_found"), { + description: t("state_not_found_in_system", { state: stateName }) + }); + } else if (stateOrg && districtName && !districtOrg) { + toast.error(t("district_not_found"), { + description: t("district_not_found_in_system", { district: districtName }) + }); + } } -}, [stateOrg, districtOrg]); +}, [stateOrg, districtOrg, stateName, districtName, t]);Likely invalid or redundant comment.
|
👋 Hi, @Mahendar0701, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/Facility/FacilityForm.tsx (1)
58-58: Consider strengthening the geo_organization validation.The current validation only checks for a non-empty string. Consider adding more specific validation to ensure the value is a valid JSON string representing an organization object, as indicated by the usage in the form reset logic.
- geo_organization: z.string().min(1, { message: "required" }), + geo_organization: z.string().min(1, { message: "required" }) + .refine( + (val) => { + try { + const parsed = JSON.parse(val); + return parsed && typeof parsed === "object"; + } catch { + return false; + } + }, + { message: "Invalid organization format" } + ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json(9 hunks)src/components/Facility/FacilityForm.tsx(12 hunks)src/pages/Appointments/AppointmentDetail.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pages/Appointments/AppointmentDetail.tsx
- public/locale/en.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (6)
src/components/Facility/FacilityForm.tsx (6)
79-91: LGTM! Well-structured component interface.The props interface and state management are well-organized to support both creation and editing scenarios.
508-523: LGTM! Well-implemented loading states and i18n.The button states and loading indicators are properly handled, with clear visual feedback and internationalized text.
216-222: 🛠️ Refactor suggestionPrevent race conditions in organization updates.
The effect doesn't handle the case where multiple pincode changes occur rapidly, which could lead to incorrect organization selections.
useEffect(() => { + let isActive = true; if (stateOrg && districtOrg) { + if (!isActive) return; setSelectedLevels([stateOrg, districtOrg]); } else { + if (!isActive) return; setSelectedLevels([]); } + return () => { + isActive = false; + }; }, [stateOrg, districtOrg]);Likely invalid or redundant comment.
224-227: Verify pincode auto-fill behavior.The pincode change handler is implemented, but we should verify:
- That it handles backspace/delete operations correctly
- That it doesn't trigger unnecessary API calls while typing
- That it properly clears organization data when pincode is invalid
Also applies to: 361-366
200-206: 🛠️ Refactor suggestionAdd error handling for pincode query.
The pincode query should handle API failures gracefully to improve user experience.
const { data: pincodeData } = useQuery({ queryKey: ["pincodeDetails", pincode], queryFn: () => getPincodeDetails(pincode, careConfig.govDataApiKey), + onError: (error) => { + form.setError("pincode", { + type: "manual", + message: t("error_fetching_pincode_details") + }); + }, enabled: validatePincode(pincode) && Number(pincode) !== Number(facilityData?.pincode), });Likely invalid or redundant comment.
169-169: Review geo_organization serialization logic.Double JSON serialization might occur here. If
facilityData.geo_organizationis already a string,JSON.stringifywill add extra quotes.
|
@Mahendar0701 what is the status on this PR, we need to get it wrapped ASAP |
Yeah i am working on it. |
|
👋 Hi, @Mahendar0701, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/components/Facility/FacilityForm.tsx (1)
Line range hint
111-131: Add error handling for mutations.Both create and update mutations lack error handling, which could leave users unaware of submission failures.
Add error handling to both mutations:
const { mutate: createFacility, isPending } = useMutation({ mutationFn: mutate(routes.facility.create), onSuccess: (_data: BaseFacility) => { toast.success(t("facility_added_successfully")); queryClient.invalidateQueries({ queryKey: ["organizationFacilities"] }); form.reset(); onSubmitSuccess?.(); }, + onError: (error) => { + toast.error(t("facility_creation_failed"), { + description: error instanceof Error ? error.message : t("please_try_again") + }); + } }); const { mutate: updateFacility, isPending: isUpdatePending } = useMutation({ mutationFn: mutate(routes.updateFacility, { pathParams: { id: facilityId || "" }, }), onSuccess: (_data: FacilityModel) => { toast.success(t("facility_updated_successfully")); queryClient.invalidateQueries({ queryKey: ["organizationFacilities"] }); form.reset(); onSubmitSuccess?.(); }, + onError: (error) => { + toast.error(t("facility_update_failed"), { + description: error instanceof Error ? error.message : t("please_try_again") + }); + } });
🧹 Nitpick comments (3)
src/pages/Organization/OrganizationFacilities.tsx (2)
124-128: Optimize alt text for better accessibility.While the image handling is good, consider providing a more descriptive alt text for better accessibility.
- alt={facility.name} + alt={`${facility.name} facility cover image`}
152-157: Remove unnecessary paragraph wrapper.The
<p>tag wrappingEditFacilitySheetis unnecessary and could cause accessibility issues. The hover styles can be moved to the component itself.- <p className="hover:underline text-underline-offset-[4px] text-decoration-thickness-[2px]"> - <EditFacilitySheet - organizationId={id} - facilityId={facility.id} - /> - </p> + <EditFacilitySheet + organizationId={id} + facilityId={facility.id} + className="hover:underline text-underline-offset-[4px] text-decoration-thickness-[2px]" + />src/components/Facility/FacilityForm.tsx (1)
Line range hint
419-440: Improve accessibility for interactive elements.The location button and other interactive elements lack proper accessibility attributes.
Add accessibility improvements:
<Button type="button" variant="outline" size="sm" onClick={handleGetCurrentLocation} disabled={isGettingLocation} className="flex items-center gap-2" data-cy="get-location-button" + aria-label={t("get_current_location")} + aria-busy={isGettingLocation} > {isGettingLocation ? ( <> - <CareIcon icon="l-spinner" className="h-4 w-4 animate-spin" /> + <CareIcon + icon="l-spinner" + className="h-4 w-4 animate-spin" + aria-hidden="true" + /> {t("getting_location")} </> ) : ( <> - <CareIcon icon="l-location-point" className="h-4 w-4" /> + <CareIcon + icon="l-location-point" + className="h-4 w-4" + aria-hidden="true" + /> {t("get_current_location")} </> )} </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/Routers/routes/FacilityRoutes.tsx(1 hunks)src/components/Facility/FacilityCreate.tsx(0 hunks)src/components/Facility/FacilityForm.tsx(12 hunks)src/components/Facility/FacilityHome.tsx(2 hunks)src/pages/Organization/OrganizationFacilities.tsx(2 hunks)src/pages/Organization/components/EditFacilitySheet.tsx(1 hunks)src/pages/Organization/components/OrganizationSelector.tsx(4 hunks)src/types/facility/facility.ts(2 hunks)
💤 Files with no reviewable changes (1)
- src/components/Facility/FacilityCreate.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- src/types/facility/facility.ts
- src/pages/Organization/components/EditFacilitySheet.tsx
- src/pages/Organization/components/OrganizationSelector.tsx
- src/components/Facility/FacilityHome.tsx
🔇 Additional comments (8)
src/Routers/routes/FacilityRoutes.tsx (2)
Line range hint
14-31: LGTM! Clean and consistent route definitions.The route definitions follow a consistent pattern and are properly typed with
AppRoutes. The organization of routes is logical, grouping related facility functionalities together.
14-17: Verify handling of old route URLs.The architectural change from dedicated create/update pages to in-page editing via
FacilityHomeis a good improvement. However, we should ensure proper handling of any bookmarked URLs for the removed routes (/facility/createand/facility/:facilityId/update).Let's check for any remaining references to the old routes:
✅ Verification successful
Route changes verified - no remaining references found
The removal of
/facility/createand/facility/:facilityId/updateroutes is safe. No existing code references these routes, and all found update-related paths belong to different features (patients, encounters, consultations). The architectural change to use in-page editing is clean and won't affect any existing navigation code.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to the removed routes in the codebase echo "Searching for references to removed routes..." rg -i "facility/create|facility/.+/update" --type ts --type tsx # Search for potential navigation code that might be affected echo "Searching for potential navigation code..." rg "navigate\(.+facility" --type ts --type tsxLength of output: 366
Script:
#!/bin/bash # Search for references to removed routes without file type restrictions echo "Searching for references to removed routes..." rg "facility/create|facility/.+/update" # Use ast-grep to find navigation calls related to facility routes echo "Searching for navigation patterns..." ast-grep --pattern 'navigate($$$facility$$$)' # Search for potential route string definitions echo "Searching for route string patterns..." rg "['\"]/facility/.*?['\"]"Length of output: 5074
src/pages/Organization/OrganizationFacilities.tsx (4)
20-20: LGTM! Import aligns with the new editing feature.The import of
EditFacilitySheetis correctly placed and follows the project's import organization pattern.
118-120: Key prop added as recommended.The Card component now correctly includes the unique key prop using facility.id.
160-173: Improve accessibility of the facility link.The current implementation of the view facility link could be improved for better accessibility.
- <Button - variant="link" - size="icon" - className="text-primary" - asChild - > - <Link - href={`/facility/${facility.id}`} - className="text-sm w-full hover:underline" - > - {t("view_facility")} - <CareIcon icon="l-arrow-up-right" className="h-4 w-4" /> - </Link> - </Button> + <Button + variant="link" + size="sm" + className="text-primary hover:underline gap-2" + asChild + > + <Link href={`/facility/${facility.id}`}> + {t("view_facility")} + <CareIcon icon="l-arrow-up-right" className="h-4 w-4" /> + </Link> + </Button>
153-156: Verify auto-population functionality.Please ensure that the
EditFacilitySheetcomponent correctly implements the auto-population of pincode and geo_organization information as specified in the PR objectives.✅ Verification successful
Auto-population functionality verified successfully.
The implementation correctly handles auto-population of pincode and geo_organization information:
- Pincode changes trigger automatic fetching of location details
- State and district organizations are automatically populated based on pincode
- Proper error handling and validation are in place
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if EditFacilitySheet handles auto-population # Check for pincode and geo_organization related code echo "Checking for pincode and geo_organization handling in EditFacilitySheet..." rg -A 5 "pincode|geo_organization" "./src/pages/Organization/components/EditFacilitySheet.tsx" # Check for form field population logic echo "Checking for form field population logic..." rg -A 5 "defaultValues|initialValues|setValue" "./src/pages/Organization/components/EditFacilitySheet.tsx"Length of output: 457
Script:
#!/bin/bash # First, find the EditFacilitySheet component file echo "Searching for EditFacilitySheet component..." fd "EditFacilitySheet" --type f # If found, let's look at its content for form handling echo "Checking component implementation..." rg -l "EditFacilitySheet" --type tsxLength of output: 334
Script:
#!/bin/bash # Get the complete implementation of EditFacilitySheet echo "Examining EditFacilitySheet implementation..." cat src/pages/Organization/components/EditFacilitySheet.tsx # Also check for any API or data fetching logic echo -e "\nChecking for data fetching patterns..." rg -A 5 "useQuery|fetch|get.*Facility" src/pages/Organization/components/EditFacilitySheet.tsxLength of output: 1985
Script:
#!/bin/bash # Examine FacilityForm implementation echo "Checking FacilityForm implementation..." cat src/components/Facility/FacilityForm.tsx # Look for specific auto-population logic echo -e "\nChecking for auto-population patterns..." rg -A 5 "pincode|geo_organization|defaultValues|useEffect.*facility" src/components/Facility/FacilityForm.tsxLength of output: 21014
src/components/Facility/FacilityForm.tsx (2)
204-226: 🛠️ Refactor suggestionOptimize pincode and organization selection logic.
The current implementation has several potential issues:
- No debouncing on pincode changes could lead to excessive API calls
- No loading states for organization selection
- Potential race conditions in the organization selection effect
Consider these improvements:
+ const [debouncedPincode] = useDebounce(pincode, 500); const { data: pincodeData, isLoading: isPincodeLoading } = useQuery({ - queryKey: ["pincodeDetails", pincode], + queryKey: ["pincodeDetails", debouncedPincode], queryFn: () => getPincodeDetails(pincode, careConfig.govDataApiKey), enabled: validatePincode(pincode) && Number(pincode) !== Number(facilityData?.pincode), }); useEffect(() => { + let isSubscribed = true; if (stateOrg && districtOrg) { + if (!isSubscribed) return; setSelectedLevels([stateOrg, districtOrg]); } else { + if (!isSubscribed) return; setSelectedLevels([]); } + return () => { + isSubscribed = false; + }; }, [stateOrg, districtOrg]);Likely invalid or redundant comment.
80-84: Add validation for mutually exclusive props.The component accepts both
organizationIdandfacilityIdas optional props, but there's no validation to ensure they aren't provided simultaneously or that at least one is provided.Consider adding runtime validation:
interface FacilityProps { organizationId?: string; facilityId?: string; onSubmitSuccess?: () => void; } export default function FacilityForm(props: FacilityProps) { + if (props.organizationId && props.facilityId) { + throw new Error("Cannot provide both organizationId and facilityId"); + } + if (!props.organizationId && !props.facilityId) { + throw new Error("Must provide either organizationId or facilityId"); + }
|
|
||
| const facilityFormSchema = z.object({ | ||
| facility_type: z.string().min(1, "Facility type is required"), | ||
| name: z.string().min(1, "Name is required"), | ||
| description: z.string().optional(), | ||
| features: z.array(z.number()).default([]), | ||
| pincode: z.string().refine(validatePincode, "Invalid pincode"), | ||
| geo_organization: z.string().min(1, t("organization_required")).optional(), |
There was a problem hiding this comment.
Resolve inconsistent validation for geo_organization field.
The schema validation for geo_organization is marked as optional but includes a required validation message. Additionally, based on the type hints and usage, this should validate an Organization object rather than a string.
Consider updating the validation:
- geo_organization: z.string().min(1, t("organization_required")).optional(),
+ geo_organization: z.object({
+ id: z.string(),
+ name: z.string(),
+ }).optional().or(z.string().min(1, t("organization_required")))Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/components/Facility/FacilityForm.tsx (1)
133-139: Add loading state handling for facility dataThe component should show a loading state while facility data is being fetched.
Consider adding:
- const { data: facilityData } = useQuery({ + const { data: facilityData, isLoading: isFacilityLoading } = useQuery({And use it to show a loading indicator or disable the form while loading.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(9 hunks)src/components/Facility/FacilityForm.tsx(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- public/locale/en.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (5)
src/components/Facility/FacilityForm.tsx (5)
80-92: LGTM! Clean interface and state managementThe component's interface and initialization are well-structured, with clear separation of concerns and proper typing.
Line range hint
182-203: LGTM! Robust location handlingThe location handling implementation includes proper error handling, user feedback, and timeout configuration.
520-544: LGTM! Well-structured form submission UIThe form submission UI handles different states (create/update) cleanly with proper loading indicators and translations.
204-210: 🛠️ Refactor suggestionAdd error handling for pincode API
The pincode query should handle API failures gracefully.
Apply this diff:
const { data: pincodeData } = useQuery({ queryKey: ["pincodeDetails", pincode], queryFn: () => getPincodeDetails(pincode, careConfig.govDataApiKey), + onError: (error) => { + toast.error(t("error_fetching_pincode_details")); + }, enabled: validatePincode(pincode) && Number(pincode) !== Number(facilityData?.pincode),Likely invalid or redundant comment.
59-59:⚠️ Potential issueFix inconsistent validation for
geo_organizationfieldThe schema validation for
geo_organizationis marked as optional but includes a required validation message. This could lead to validation errors when the field is intentionally left empty.Apply this diff to fix the validation:
- geo_organization: z.string().min(1, t("organization_required")).optional(), + geo_organization: z.string().optional().or(z.string().min(1, t("organization_required"))),Likely invalid or redundant comment.
Proposed Changes
Fixes Switch to using Create Facility Form #9849
Pincode autofill and geo_organization info auto populates
Added edit feature for facility in orgainization
Added support for organization selector in facility form
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Release Notes
New Features
Improvements
Localization
The release introduces more flexible facility management with enhanced location and organization selection capabilities, along with improved user experience in editing facilities.